Skip to content

Add Banner to README file#373

Merged
jdevera merged 4 commits intopythoncanarias:masterfrom
cesaralvrz:master
Oct 3, 2021
Merged

Add Banner to README file#373
jdevera merged 4 commits intopythoncanarias:masterfrom
cesaralvrz:master

Conversation

@cesaralvrz
Copy link
Copy Markdown
Contributor

@cesaralvrz cesaralvrz commented Oct 2, 2021

I added the banner to the README file with a title with align to the center.

Also fixed a broken error for the website of Python Canarias.

@jdevera jdevera self-requested a review October 2, 2021 10:32
Copy link
Copy Markdown
Contributor

@jdevera jdevera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for this first contribution! 🎉 🎉 🎉

Also, really nice banner! 😍

I left a comment about the image URL, and here are some more that I could not attach to the right file because github will not let me comment on whole files:

  • Please rename the PyCa.png to something more descriptive, such as python_canarias_banner.png (I changed the casing here just for personal preference, it's not important as long as it does not have spaces in the name)
  • Please move the assets folder to be under the docs folder, this makes it extra clear it's not part of the app. Note this will require changing the path in the README too.

One meta topic: This PR comes from a conversation somewhere else and I have that context, so it's fine, but generally it is best to also include the motivation of the PR in the description itself. Not only for the reviewers of now but also for future reference.

And one topic for discussion: I noticed the banner image is larger than it ever renders in Github. Github caps this as 838px in width no matter how much bigger I make the viewport.

I think we should optimise this in a repository that is otherwise used for coding an app. If your original work on the banner comes from a vector graphics file such as SVG, It'd be great to also contribute that to the docs repository, while the image we store in this repo is the one optimised for its current use: show up in the README. What do you think?

Comment thread README.md Outdated

Website of [Python Canarias](pythoncanarias.es) 🚀   happily made with [Django](https://www.djangoproject.com/).

![Python Canarias Banner](https://github.com/cesaralvrz/pycan-web/blob/master/assets/PyCa.png)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The banner image URL points to your own fork. Once this PR is merged, it would continue to point to your fork. For images that are also hosted in the repositories, the preference is to use relative paths to them. In this case, this would look like this:

Suggested change
![Python Canarias Banner](https://github.com/cesaralvrz/pycan-web/blob/master/assets/PyCa.png)
![Python Canarias Banner](assets/PyCa.png)

@cesaralvrz
Copy link
Copy Markdown
Contributor Author

The original work is also in PNG, but if you want I can add the banner in the original size to the docs repository.

@cesaralvrz cesaralvrz requested a review from jdevera October 2, 2021 14:44
Copy link
Copy Markdown
Contributor

@jdevera jdevera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the additional changes! Looks great!

@jdevera jdevera merged commit 7bdc810 into pythoncanarias:master Oct 3, 2021
@sdelquin
Copy link
Copy Markdown
Contributor

sdelquin commented Oct 3, 2021

Gracias @cesaralvrz por la colaboración altruista y gracias @jdevera por revisarlo y mergearlo 😄

@euribates euribates added hacktoberfest Aptas para Hacktoberfest hacktoberfest-accepted HacktoberFest Accepted labels Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest Aptas para Hacktoberfest hacktoberfest-accepted HacktoberFest Accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants